-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ui: Use DataSources in ACLs area #7681
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kaxcode
approved these changes
Apr 22, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
♻️ LGTM
johncowen
added a commit
that referenced
this pull request
Apr 30, 2020
* ui: Use Datasource for loading related data in ACLs area * ui: Use more manual cleanup for Controller event-sources * Update reconcile to use nspace and add SyncTime to role/policy * Use the correct value for nspace and dc (the one from the item itself) * Remove the // check, we no longer need it. Add some TODO
kaxcode
pushed a commit
that referenced
this pull request
May 11, 2020
* ui: Use Datasource for loading related data in ACLs area * ui: Use more manual cleanup for Controller event-sources * Update reconcile to use nspace and add SyncTime to role/policy * Use the correct value for nspace and dc (the one from the item itself) * Remove the // check, we no longer need it. Add some TODO
johncowen
added a commit
that referenced
this pull request
May 12, 2020
* ui: Use Datasource for loading related data in ACLs area * ui: Use more manual cleanup for Controller event-sources * Update reconcile to use nspace and add SyncTime to role/policy * Use the correct value for nspace and dc (the one from the item itself) * Remove the // check, we no longer need it. Add some TODO
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following #7334 we noticed that our event sources were no longer being cleanup correctly.
Our new style
DataSources
use Ember Components instead of Ember Proxies and are much more flexible and simpler to use. The main difference beingDataSources
automatically clean up when they are removed from the page or removed fromdisplay
(depending if they useloading="lazy"
or not)This uses
DataSources
in the ACLs area, mainly to load in datacenters in the forms/modals where we use them - and is partly where the idea forIntersectionObserver
backedDataSource
s came from.A few more advantages here:
Datasource
/Blocking query support keeps BlockingQueries open until they time out, or we max out our HTTP request restriction. That means they are reused between different instances of the same component in a page, or potentially across pages. This makes for instantaneous loading in some areas.There is one place where we continue to use our old style EventSources, which we plan to swap out for our new
<DataSink />
component further down the line.We unfortunately and very reluctantly commented out an assertion in a test here. The assertion depends on the
DataSource
being within the viewport, which you can never guarantee in our test runner (due to skipped tests and size of the window etc etc).We tried a few methods to improve this mainly around automatically scrolling to about to be clicked dom elements - following the thinking that in order to click something in must first be in the viewport. In the end we decided that this wasn't the right way to solve the problem, and it would be better to somehow provide mockable functionality for
dom.isInViewport
, and potentially a teststep
to 'pretend' that aDataSource
is in the viewport.The latter approach probably needs more in-depth work and before that, thinking through properly. So we went with the comment in order to move forwards and come back at a later date 😿